Recognise PersistentBase and XPBaseObject as persistent bases - #10
Conversation
peopleworks
left a comment
peopleworks
left a comment
There was a problem hiding this comment.
Approving, and this one taught me something about my own issue.
I filed #7 as a duplication problem: one list written in four places, plus a default that agreed with none of them. You fixed that — the four copies are gone and callers now get the default — and then noticed the thing I had not, which is that the list itself was wrong in the middle.
PersistentBase → XPBaseObject → XPCustomObject → XPObject
→ XPLiteObject
We recognised the leaves and not the two classes above them. I had looked straight at that array while writing #7 and read it as a set of names to deduplicate rather than as a hierarchy with a hole in it. Your framing is the one that makes it obvious:
mapping onto a table that brings its own key is exactly when the higher bases are the right choice
Which is the same population every fix this week has been about. An application on an existing schema is precisely the one that will derive from XPBaseObject or PersistentBase, and it was the one we could not see.
The two fixtures are the right two: AuditEntry : XPBaseObject supplying its own [Key] because the base carries none, and SequenceCounter : PersistentBase on a natural string key. Both are shapes a real legacy module has, not shapes invented to make a test pass.
Verified in a clean worktree, Windows, .NET 10, Release:
Passed! Failed: 0, Passed: 280, Skipped: 0 0 warnings, 0 errors
I checked the over-reporting direction too, since widening a seed list is exactly where that risk lives, and more so once #9's base walk propagates from these roots. Every one of the five names is documented by DevExpress as a base a persistent class may derive from, so anything reaching them is persistent by definition. No note needed — I am recording that I looked.
Closes #7. Thank you for taking an issue that was mine to fix.
One practical thing
You and #9 both edit README.md, CHANGELOG.md and SampleProjects.cs, and you claim different test counts — 280 here, 283 there. Whichever merges second will conflict and then fail CountsTheTestsThatActuallyExist, which reads the README claim against the real suite. Not a problem with either change, just an order-of-merge tax: rebase the second and fix the number.
If you have a preference, merge this one first — it is the smaller rebase, and #9's base walk is more interesting to read against a seed list that is already correct.
0d60611 to
1db064c
Compare
|
Rebased onto What I resolved:
Nothing of yours changed. Shout if you would rather have done it yourself and I will keep my hands off the next one. One thing you should know before it surprises you: force-pushing dismissed my approval, because |
The XPO hierarchy is PersistentBase -> XPBaseObject -> XPCustomObject -> XPObject, with XPLiteObject also under XPBaseObject. BaseTypeNames held the three leaves and neither of the classes above them, so the hole sat in the middle of an API that is used rather than at its edge: DevExpress documents all five as bases a persistent class may derive from, and recommends PersistentBase. Deriving from the higher bases is what you do when the table already brings its own key -- the same population of legacy schemas the DbSet roster was added for, which is why this turned up beside it. FeatureCenter.NET.XPO gains OidGenerator, NoKeyPropertyNamedBaseObject and LayoutDemoObject, all of them ": XPBaseObject". Nothing changes for MainDemo in either ORM, for OutlookInspired, or for a private 221-entity EF Core application. No demo derives from PersistentBase directly, so that half is covered by the fixture rather than by a corpus. The list also stops being duplicated. The CLI, the MCP server and the test harness each passed their own copy, so the default in Core was four names while every caller passed five -- four copies and three chances for them to disagree about what an entity is. They now use the default. Fixes peopleworks#6 in part; the transitive walk in peopleworks#9 is the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fifth copy, found by peopleworks in peopleworks#7. Leaving it would have kept the sync path on the old list while every other caller moved, which is the same divergence one copy further along. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1db064c to
826cea0
Compare
3 participants
What this changes
PersistentBaseandXPBaseObjectjoinBaseTypeNames, and the list stops being duplicated across the CLI, the MCP server and the test harness.Why
I said in #6 this was one word. It is two, and the second one is the interesting one.
The XPO hierarchy, per the class comparison page:
The list held
XPCustomObject,XPObjectandXPLiteObject— the three leaves — and neither of the two classes above them. So the hole was in the middle of a documented API rather than at its edge: DevExpress names all five as bases a persistent class may derive from, and the same page says "We recommend that you use thePersistentBaseclass as the base class for persistent classes in your applications."It is also not an exotic corner.
XPBaseObjectandPersistentBasecarry no built-in key, which is exactly why you pick them when the table already has one — the same population of legacy schemas the DbSet roster was added for in #3. That is presumably why this surfaced beside it.The duplication, which is the part I got wrong before
I claimed on #4 that
BaseTypeNamesdefaults to four names "the CLI never uses" and thatApplicationUserwas therefore missed in the field. That was wrong and I corrected it — but the thing underneath it is real, just in the other direction: the list exists in four places.ExtractionOptionsdefaultCli/Program.csMcp/XafProjectContext.csSampleProjects.ExtractThree copies agreed with each other and the default disagreed with all of them, which is what made me misread it. Four copies is three chances for them to drift, and the drift is silent — each one is just a different opinion about what an entity is. The callers now use the default, so the change is
-3duplicated arrays and one list to edit next time.If you would rather keep the call sites explicit, say so and I will put the two names in all four instead; the fix works either way and I do not want to smuggle a refactor into a bug fix.
Verification
FeatureCenter.NET.XPOMainDemo.NET.XPOMainDemo.NET.EFCoreOutlookInspiredDemo.NET.EFCoreThe three FeatureCenter additions are
OidGenerator,NoKeyPropertyNamedBaseObjectandLayoutDemoObject, all: XPBaseObject— the last from theFeatureCenter.Module.Winsibling project.No demo derives from
PersistentBasedirectly, so that half is covered by the fixture and not by a corpus. Worth knowing rather than implying both halves were proven in the field.Numbers are against
main, so they do not include #9; the two changes are independent and address different causes from #6.Three existing assertions moved, deliberately
The XPO fixture gained
AuditEntry : XPBaseObjectandSequenceCounter : PersistentBase, because the canonical XPO sample having no class on either base is why the hole was invisible. That changes three counts, and I would rather point at them than have them found:ExtractionTests.FindsEveryEntity— expected set now names all fiveHtmlExplainerTests.GraphPlacesEveryEntity—3→5nodesMcpToolTests.AbsentEntityReturnsTheCompleteInventoryAndSaysItDoesNotExist—"complete list of 3 entities"→5Each is a count of what the fixture holds, and each still asserts exactly what it asserted before. If you would rather the fixture stayed at three entities, the two classes can move to their own solution folder instead.
280 passed, 0 failed,dotnet build XAFLogicExplainer.slnxclean at 0 warnings. The new test fails onmainfor the right reason.Fixes the second of the three causes in #6.
Checklist
dotnet build XAFLogicExplainer.slnxis clean (CI treats warnings as errors)XafLogicExplainer.CoreCHANGELOG.mdupdated under[Unreleased]